Conversation
…etric dimension First of a stacked series splitting the PubSub Health Monitoring feature (linkedin#2534) into reviewable chunks. Introduces only the foundational, behavior-free building blocks — no wiring, all default-off. - New pubsub health types (venice-common): - PubSubHealthStatus (HEALTHY/UNHEALTHY) - PubSubHealthCategory (BROKER/METADATA_SERVICE) as a metric dimension - PubSubHealthSignalProvider (pluggable detection interface) - PubSubHealthChangeListener (state-transition callback interface) - New config keys (all default-off) + VeniceServerConfig parsing: - server.pubsub.health.monitor.enabled - server.pubsub.partition.pause.enabled - server.pubsub.health.probe.interval.seconds - server.pubsub.health.probe.topic - New OTel dimension VENICE_PUBSUB_HEALTH_CATEGORY (required by PubSubHealthCategory) + dimension unit tests. No call sites yet; compiles and tests independently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 task
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces foundational (behavior-free) building blocks for upcoming PubSub Health Monitoring work: core health types, server config keys + parsing, and a new OpenTelemetry metric dimension to tag health category.
Changes:
- Added core PubSub health enums/interfaces (
PubSubHealthStatus,PubSubHealthCategory,PubSubHealthSignalProvider,PubSubHealthChangeListener) invenice-common. - Introduced new server config keys in
ConfigKeysand parsed/exposed them viaVeniceServerConfiggetters (all default-off). - Added new OTel dimension
VENICE_PUBSUB_HEALTH_CATEGORYplus unit tests covering dimension naming and enum-to-dimension behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/venice-common/src/main/java/com/linkedin/venice/pubsub/PubSubHealthStatus.java | Adds enum for HEALTHY/UNHEALTHY status. |
| internal/venice-common/src/main/java/com/linkedin/venice/pubsub/PubSubHealthSignalProvider.java | Adds pluggable interface for providing PubSub health signals. |
| internal/venice-common/src/main/java/com/linkedin/venice/pubsub/PubSubHealthChangeListener.java | Adds callback interface for health state transitions. |
| internal/venice-common/src/main/java/com/linkedin/venice/pubsub/PubSubHealthCategory.java | Adds enum for health categories and binds it to an OTel dimension. |
| internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java | Adds new PubSub health monitoring config keys (default-off). |
| internal/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensions.java | Adds new OTel dimension constant for PubSub health category. |
| internal/venice-client-common/src/test/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensionsTest.java | Extends dimension naming tests to cover the new dimension. |
| internal/venice-client-common/src/test/java/com/linkedin/venice/stats/dimensions/PubSubHealthCategoryTest.java | Adds tests for PubSubHealthCategory implementing the dimension interface. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/config/VeniceServerConfig.java | Parses and exposes new PubSub health-related server configs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** | ||
| * A pluggable provider of health signals for PubSub targets. The PubSub health monitor | ||
| * aggregates signals from all registered providers to determine overall broker health. |
configs Covers the diffCoverage gate for da-vinci-client: exercises the new pubsub-health config getters and both sides of the `pubSubHealthMonitorEnabled && ...` short-circuit in partition-pause resolution (monitor-on honors pause, monitor-off gates it off), plus the default values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/PubSubHealthSignalProvider.java:6
- The class-level Javadoc says the monitor "determine[s] overall broker health", but this provider API is for PubSub targets across categories (including METADATA_SERVICE). The wording should be target/category-neutral to avoid misleading future implementers.
* A pluggable provider of health signals for PubSub targets. The PubSub health monitor
* aggregates signals from all registered providers to determine overall broker health.
*
Comment on lines
+191
to
+192
| /** {@link com.linkedin.venice.pubsub.PubSubHealthCategory} */ | ||
| VENICE_PUBSUB_HEALTH_CATEGORY("venice.pubsub.health.category"); |
|
Hi there. This pull request has been inactive for 30 days. To keep our review queue healthy, we plan to close it in 7 days unless there is new activity. If you are still working on this, please push a commit, leave a comment, or convert it to draft to signal intent. Thank you for your time and contributions. |
|
Closing this pull request due to 37 days of inactivity. This is not a judgment on the value of the work. If you would like to continue, please reopen or open a new PR and we will be happy to take another look. Thank you again for contributing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of a stacked series splitting the PubSub Health Monitoring feature (#2534) into smaller reviewable chunks. This PR introduces only the foundational, behavior-free building blocks — no wiring, no call sites, all default-off.
What's included
New pubsub health types (
venice-common):PubSubHealthStatus(HEALTHY / UNHEALTHY)PubSubHealthCategory(BROKER / METADATA_SERVICE) — also a metric dimensionPubSubHealthSignalProvider— pluggable detection interfacePubSubHealthChangeListener— state-transition callback interfaceNew config keys (all default-off) +
VeniceServerConfigparsing:server.pubsub.health.monitor.enabledfalseserver.pubsub.partition.pause.enabledfalseserver.pubsub.health.probe.interval.seconds30server.pubsub.health.probe.topic""New OTel dimension
VENICE_PUBSUB_HEALTH_CATEGORY(required byPubSubHealthCategory) + dimension unit tests.Stack
This is PR 1 of a series that decomposes #2534:
PubSubHealthMonitor+ signal provider + stats)How was this PR tested?
venice-common,venice-client-common,da-vinci-client).VeniceMetricsDimensionsTest,PubSubHealthCategoryTestpass.Does this PR introduce any user-facing or breaking changes?